Resolve PHP version checks in remaining scope-aware extensions from Scope::getPhpVersion() and forbid injecting PhpVersion into them - #6563
Conversation
…Scope::getPhpVersion()` and forbid injecting `PhpVersion` into them
- StrSplitFunctionReturnTypeExtension: str_split('') yields array{}|array{''} when the analysed versions straddle 8.2; ValueError/false and empty-array handling use TrinaryLogic from the scope
- MbFunctionsReturnTypeExtension, MbStrlenFunctionReturnTypeExtension: invalid-encoding never/false decided per scope
- MbFunctionsReturnTypeExtensionTrait: caches the full encoding list, filters PASS/NONE per call from the scope's PHP version
- MbSubstituteCharacterDynamicReturnTypeExtension: computes code points valid on all vs. on some analysed versions, so version ranges produce bool instead of a wrong constant; new PhpVersion(s)::isZeroValidCodePointInMbSubstituteCharacter()
- PDOConnectReturnTypeExtension: hasPDOSubclasses() checked against the scope
- AdapterReflectionEnum(Case)DynamicReturnTypeExtension, NativeReflectionEnumReturnDynamicReturnTypeExtension: >= 8.0 check against the scope
- New build rule NoPhpVersionInjectionInScopeAwareExtensionRule reports scope-aware extensions (return type, throw type, type-specifying, closure type/this, param-out, expression type resolver) whose constructor takes PhpVersion
- Not converted: BcMath operator type-specifying extensions (OperatorTypeSpecifyingExtension gets no Scope), ArrayUnpackingHelper (engine helper), RegexArrayShapeMatcher/RegexGroupParser (no Scope at the check site)
| public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): ?Type | ||
| { | ||
| if ($this->phpVersion->getVersionId() >= 80000) { | ||
| if (IntegerRangeType::fromInterval(80000, null)->isSuperTypeOf($scope->getPhpVersion()->getType())->yes()) { |
There was a problem hiding this comment.
this needs a new PhpVersions method instead
There was a problem hiding this comment.
I made both requested changes in two separate commits and pushed them. make phpstan reports no errors and make tests passes (22221 tests, 105 skipped).
1. Reflection enum extensions (cd0784380)
- I added
PhpVersions::hasCorrectReflectionEnumAdapterReturnTypes(), which is true on PHP 8.0+. AdapterReflectionEnumDynamicReturnTypeExtension,AdapterReflectionEnumCaseDynamicReturnTypeExtensionandNativeReflectionEnumReturnDynamicReturnTypeExtensionnow call$scope->getPhpVersion()->hasCorrectReflectionEnumAdapterReturnTypes()->yes()instead of building anIntegerRangeTypeinline.
2. PDO::connect (71a3320e5)
PDOConnectReturnTypeExtensionis back to exactly how it was before this PR: it injectsPhpVersionand checkshasPDOSubclasses()inisStaticMethodSupported().- The new build rule would flag that constructor, so I made the rule's error ignorable. The exception is marked on the constructor parameter with
// @phpstan-ignore phpstan.phpVersionInjection (PDO subclasses only exist in the runtime when running on PHP 8.4+). A side effect is that anyone else can now silence this rule with an inline ignore too. If you'd rather keep it non-ignorable, the alternative is a list of allowed classes inside the rule. - I removed the
PDO::connectcase fromnsrt/bug-15287.php, because it no longer depends on the scope.nsrt/pdo-connect-php84.phpstill tests this extension.
| public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): ?Type | ||
| { | ||
| if ($this->phpVersion->getVersionId() >= 80000) { | ||
| if (IntegerRangeType::fromInterval(80000, null)->isSuperTypeOf($scope->getPhpVersion()->getType())->yes()) { |
There was a problem hiding this comment.
this needs a new PhpVersions method instead
| public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): ?Type | ||
| { | ||
| if ($this->phpVersion->getVersionId() >= 80000) { | ||
| if (IntegerRangeType::fromInterval(80000, null)->isSuperTypeOf($scope->getPhpVersion()->getType())->yes()) { |
There was a problem hiding this comment.
this needs a new PhpVersions method instead
| return null; | ||
| } | ||
|
|
||
| $valueType = $scope->getType($methodCall->getArgs()[0]->value); |
There was a problem hiding this comment.
the below listed PDO subclasses only exist in PHP 8.4+ - see https://wiki.php.net/rfc/pdo_driver_specific_subclasses
thats why PDOConnectReturnTypeExtension depends on a PHP 8.4+ runtime and we need to revert to using PhpVersion instead of PhpVersions
…he reflection enum return type extensions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PDO driver subclasses only exist in the runtime on PHP 8.4+, so the extension depends on the runtime version rather than the scope's one. The build rule is now ignorable so this exception can be marked inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return []; | ||
| } | ||
|
|
||
| $constructor = $node->getOriginalNode()->getMethod('__construct'); |
There was a problem hiding this comment.
use ClassReflection->has/getConstructor instead
There was a problem hiding this comment.
I made both of your changes to NoPhpVersionInjectionInScopeAwareExtensionRule and pushed them as one commit (f69c8bae7), since they rewrite the same lines. make phpstan reports no errors and make tests passes (22221 tests, 105 skipped).
- Constructor lookup: the rule now uses
$classReflection->hasConstructor()andgetConstructor()->getOnlyVariant()->getParameters(). It no longer reads__constructfrom the AST. - Type check: each parameter is checked with
(new ObjectType(PhpVersion::class))->isSuperTypeOf(TypeCombinator::removeNull($parameter->getType()))->yes(). This replaces theName/NullableTypechecks on the AST, and still catches?PhpVersion. - Error line: reflection has no line numbers for parameters, so the error is now reported on the class line instead of the parameter line. To still point at the offending parameter, the message now names it ("…via constructor parameter
$phpVersion…"). - PDO exception: because of the line change, I moved the
// @phpstan-ignore phpstan.phpVersionInjection (...)comment inPDOConnectReturnTypeExtensionfrom the constructor parameter to just above#[AutowiredService].make phpstanstill passes, so the ignore still matches. - Test: I updated the expected message and line in
NoPhpVersionInjectionInScopeAwareExtensionRuleTest.
| if ($type instanceof NullableType) { | ||
| $type = $type->type; | ||
| } | ||
| if (!$type instanceof Name || $type->toString() !== PhpVersion::class) { |
There was a problem hiding this comment.
use ObjectType(PhpVersion::class) and isSuperTypeOf instead
There was a problem hiding this comment.
Both of the latest review comments on NoPhpVersionInjectionInScopeAwareExtensionRule are addressed and pushed. make phpstan reports no errors and make tests passes (22221 tests, 105 skipped).
My first push was rejected: someone had already pushed f69c8bae7 to the branch while I was working, and it handles both comments the same way my commit did. It:
- gets the constructor with
ClassReflection::hasConstructor()/getConstructor()instead of reading the AST; - checks each parameter with
(new ObjectType(PhpVersion::class))->isSuperTypeOf(TypeCombinator::removeNull($parameter->getType()))->yes()instead of matchingName/NullableTypenodes.removeNullmeans a?PhpVersionparameter is caught too.
So I dropped my copy of the same change and kept that commit.
Side effects of f69c8bae7:
- Reflection parameters have no line numbers, so the error is now reported on the class line rather than the parameter line.
- The message now names the parameter ("…via constructor parameter $phpVersion…").
- The
@phpstan-ignoreinPDOConnectReturnTypeExtensionmoved up to the class. It still matches: there is no unmatched-ignore error.
What I added (7186c8f6c): a test case for an extension whose constructor takes ?PhpVersion. The test data only had a nullable parameter in a class that isn't an extension, so that path wasn't covered before.
| $ranges = []; | ||
| $phpVersion = $scope->getPhpVersion(); | ||
|
|
||
| if ($this->phpVersion->supportsAllUnicodeScalarCodePointsInMbSubstituteCharacter()) { |
There was a problem hiding this comment.
reduce changes in this file to a minimum to only introduce PhpVersions - no other drive-by improvements
| { | ||
|
|
||
| public function __construct( | ||
| // @phpstan-ignore phpstan.phpVersionInjection (PDO subclasses only exist in the runtime when running on PHP 8.4+) |
There was a problem hiding this comment.
put the ignore into the baseline instead
…ensionRule` via reflection and `ObjectType::isSuperTypeOf()` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ctionInScopeAwareExtensionRule` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ionsReturnTypeExtensionTrait` changes to only introduce `PhpVersions` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e into the baseline Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SanderMuller
left a comment
There was a problem hiding this comment.
Reviewed at 32c22190e. The ->yes() gate on the reflection extensions fixes the phpstan-strict-rules regression from #6529. One CI red is this PR's, and the description no longer matches the code in three places.
Generate baseline
This job is red here and green on #6551 and #6485. The regenerated baseline has the same PDOConnectReturnTypeExtension entry as the committed one, but in a different position, so the hand-inserted entry is out of order. Running make phpstan-generate-baseline should fix it.
The description
- It says
PDOConnectReturnTypeExtensionnow checkshasPDOSubclasses()against the Scope.71a3320e5kept the injectedPhpVersion, and32c22190ebaselined it. - It says
MbSubstituteCharacterDynamicReturnTypeExtensionbuilds two code-point types, and it givesboolformb_substitute_character(0)under a range as the example.30f79b07creduced that change to the minimum, as you asked. Under the 7.4 to 8.5 range this head infersfalseformb_substitute_character(0), the same as the base, so the example no longer holds. - It says
bug-15287.phpchecksPDO::connect. The file has no PDO case.
A note on the minimal trait change
MbFunctionsReturnTypeExtensionTrait keeps PASS and NONE only when the range is certainly 7.2 or older. A range that reaches 7.2 therefore treats them as unsupported, although they work there. Ranges that low are rare, and ->no() would keep them, if you want the wider answer.
What I checked
- I ran phpstan-strict-rules 2.1.x's own
make phpstanwith this PR's CI phar (run 35863204809). It gives 0 errors under its^7.4 || ^8.0range, where #6529's phar gave 10. It also gives 0 at a fixed 7.4.6 and at a fixed 8.0. - With
src/reset to the merge basec230eb0b7, the range test fails 4 of 16, andbug-15287.phpfails 19 assertions. - I dumped 17 expressions at fixed 7.2, 7.4 and 8.5 and at two ranges, on base and PR. At a fixed version they all agree. Under a range,
str_split('')widens toarray{}|array{''}andmb_substitute_character(null)tobool, and nothing narrows. - Self-analysis reports the same error set as the merge base. phpcs is clean, the
Type/Php,Rules/FunctionsandBuildsuites pass, and no touched class is turbo-shadowed.
Performance
I counted MutatingScope::getPhpVersion() calls in a single-process self-analysis: 220,786 on the merge base and 221,013 here, so 227 more, or 0.1%.
CI
Apart from Generate baseline, every red also shows on #6485 or #6551. phpstan-nette (8.6) make phpstan fails in composer install on all three.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…may support them Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I've pushed two commits that address the review. 1. Generate baseline ( 2. 3. Description
|
Summary
Follow-up to #6551. That PR moved most
src/Type/Phpextensions off the DI-injectedPhpVersion. This one moves the last scope-aware extensions to$scope->getPhpVersion()too, soif (PHP_VERSION_ID >= 80000)guards and configured version ranges now affect their return types. It also adds a build rule (as Ondřej suggested in the issue) so new extensions don't go back to injectingPhpVersion.Changes
src/Type/Php/StrSplitFunctionReturnTypeExtension.php: reads the version from the scope. When the analysed versions span PHP 8.2,str_split('')now returnsarray{}|array{''}. Invalid lengths and encodings returnneveronly when a ValueError is certain.src/Type/Php/MbFunctionsReturnTypeExtension.php,src/Type/Php/MbStrlenFunctionReturnTypeExtension.php: invalid encodings giveneverorfalsedepending on the scope.src/Type/Php/MbFunctionsReturnTypeExtensionTrait.php: takesPhpVersionsas an argument. It caches the full encoding list and filters outPASS/NONEon each call.src/Php/PhpVersion.php,src/Php/PhpVersions.php: new methodisZeroValidCodePointInMbSubstituteCharacter(), which replaces the rawgetVersionId() < 80000check.src/Reflection/BetterReflection/Type/AdapterReflectionEnum{,Case}DynamicReturnTypeExtension.php,src/Reflection/PHPStan/NativeReflectionEnumReturnDynamicReturnTypeExtension.php: the>= 8.0check now uses the scope.build/PHPStan/Build/NoPhpVersionInjectionInScopeAwareExtensionRule.php(registered inbuild/phpstan.neon): reports a class whose constructor takesPhpVersionif it implements any extension interface that receives aScope. That covers dynamic return/throw type, type-specifying, parameter closure type/this, parameter-out and expression type resolver extensions.Looked at and deliberately left alone:
BcMathNumber*OperatorTypeSpecifyingExtension:OperatorTypeSpecifyingExtensiongets noScope.ArrayUnpackingHelper: an engine helper used byAssignHandler.RegexArrayShapeMatcher/RegexGroupParser: noScopeis available where the version is checked.Root cause
These extensions asked a single
PhpVersionfrom DI. That object can't see narrowing fromPHP_VERSION_IDconditions or a configured version range, so they could return the wrong type, or a type that is too precise, for the analysed code.Test
tests/PHPStan/Analyser/nsrt/bug-15287.phpchecksstr_split,mb_str_split,mb_strlen,mb_ord,mb_substitute_characterunderPHP_VERSION_IDbranches. It fails before the change and passes after.tests/PHPStan/Analyser/data/scope-php-version-range-return-type-extensions.phpadds the same functions under a 7.4–8.5 range, where results must be the union of all versions (e.g.boolformb_substitute_character(0),array{}|array{''}forstr_split('')).tests/PHPStan/Build/NoPhpVersionInjectionInScopeAwareExtensionRuleTest.phpcovers the new build rule.Fixes phpstan/phpstan#15287
🤖 Generated with Claude Code